Skip to content

Add boolean and array functions to check if the device has a sensor housing - #164

Merged
dennisweissmann merged 3 commits into
devicekit:masterfrom
Zandor300:master
Mar 4, 2019
Merged

Add boolean and array functions to check if the device has a sensor housing#164
dennisweissmann merged 3 commits into
devicekit:masterfrom
Zandor300:master

Conversation

@Zandor300

Copy link
Copy Markdown
Member

I added an array of devices that feature a sensor housing (aka a notch) and a Boolean function that tells whether or not the current device features a sensor housing.

@devicekit-danger-bot

devicekit-danger-bot commented Feb 28, 2019

Copy link
Copy Markdown
4 Warnings
⚠️ Plist changed, don’t forget to localize your plist values
⚠️ Source/Device.generated.swift#L290 - Function body should span 40 lines or less excluding comments and whitespace: currently spans 63 lines
⚠️ Source/Device.generated.swift#L607 - TODOs should be resolved (Longterm we need a better solu…).
⚠️ Source/Device.generated.swift#L1119 - Prefer empty collection over optional collection.

SwiftLint found issues

Warnings

File Line Reason
Device.generated.swift 1119 Prefer empty collection over optional collection.
Device.generated.swift 607 TODOs should be resolved (Longterm we need a better solu...).
Device.generated.swift 290 Function body should span 40 lines or less excluding comments and whitespace: currently spans 63 lines

Generated by 🚫 Danger

@Zandor300

Copy link
Copy Markdown
Member Author

@dennisweissmann what do you think of this addition?

@kudit

kudit commented Mar 3, 2019

Copy link
Copy Markdown

Didn’t review the code, and this may not apply here, but note that the iPad Pro has a “sensor housing” that is contained in the notch of iPhone X but does not have a notch.

@Zandor300

Zandor300 commented Mar 4, 2019

Copy link
Copy Markdown
Member Author

So, do you think it should be renamed to “hasNotch”? Weird, since Apple calls it a sensor housing all over it’s developer docs while I haven’t seen references like that for iPad...

@dennisweissmann

Copy link
Copy Markdown
Collaborator

Thanks for the mention @Zandor300, this triggered an email :)

I like the addition :) with regards to the naming:

I think I like “notch” better, this is the first time I heard the term sensor housing (but that’s not a good enough reason to discard the term if it’s the right one describing the property) - I’ll leave that up to you though :) in any case I’d remove the “A” in allDevicesWithA[SensorHousing | Notch].

Thank you @Zandor300 for contributing🙏 This is great :)

@Zandor300

Copy link
Copy Markdown
Member Author

@dennisweissmann can't tell if you like or dislike the email notification caused by your mention. 🤔

I think keeping it as sensor housing is the best choice:

Inset essential content to prevent clipping. In general, content should be centered and symmetrically inset so it looks great in any orientation, isn’t clipped by rounded corners, isn’t hidden by a sensor housing, and isn’t obscured by the indicator for accessing the Home screen. [...]

Don't mask or call special attention to key display features. Don't attempt to hide a device's rounded corners, sensor housing, or indicator for accessing the Home screen by placing black bars at the top and bottom of the screen. [...]

Both from https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/adaptivity-and-layout/

I will however rename the variable like you suggested.

@dennisweissmann

Copy link
Copy Markdown
Collaborator

I like the mention, sorry for not making that clearer :) I don’t know why but github doesn’t send me emails when someone opens a PR or an issue, only direct mentions cause an email ...

Alright, that seems reasonable to me, thanks for the rename and again for working on that 🎉

I’ll merge it but I can’t release it because I don’t have a MacBook right now ...

I’m also in the process of transferring this repo to a colleague, and maybe she can trigger a release sooner than I can.

Again, thank you for working on that, that’s great! :) 🙏🎉

@dennisweissmann
dennisweissmann merged commit 59f85e0 into devicekit:master Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants